-
-
Notifications
You must be signed in to change notification settings - Fork 112
Add new practice exercise split-second-stopwatch
#749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
, Ok (PreviousLaps [ "01:23:45", "04:01:40" ]) | ||
, Ok (CurrentLap "08:43:05") | ||
, Ok (Total "14:08:30") | ||
, Ok (PreviousLaps [ "01:23:45", "04:01:40", "08:43:05" ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Output
type introduced on top is only used for this exercise, it's the only one where we need to assert the values of different types. It's a little verbose, but it's the best I could come up with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to make any test failures hard to read. Not sure what the other solutions are, but maybe asserting on all the fields all the time, instead of picking out one for each assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example of a test failure (I changed total
to add 10 seconds)
↓ SplitSecondStopwatch
✗ supports very long laps
Ok (CurrentLap "01:23:45"), Ok (PreviousLaps ["01:23:45"]), Ok (CurrentLap "04:01:40"), Ok (Total "05:25:35"), Ok (PreviousLaps ["01:23:45","04:01:40"]), Ok (CurrentLap "08:43:05"), Ok (Total "14:08:40"), Ok (PreviousLaps ["01:23:45","04:01:40","08:43:05"])
╷
│ Expect.equalLists
╵
Ok (CurrentLap "01:23:45"), Ok (PreviousLaps ["01:23:45"]), Ok (CurrentLap "04:01:40"), Ok (Total "05:25:25"), Ok (PreviousLaps ["01:23:45","04:01:40"]), Ok (CurrentLap "08:43:05"), Ok (Total "14:08:30"), Ok (PreviousLaps ["01:23:45","04:01:40","08:43:05"])
The first diff is at index 3: it was `Ok (Total "05:25:35")`, but `Ok (Total "05:25:25")` was expected.
I think it doesn't read to bad, it's clear where the first problem is, and it's very clear that it's about total
. What do you think?
maybe asserting on all the fields all the time, instead of picking out one for each assertion?
I was following the problem specs as close as I could, they are only asserting those specific values, it's easier to maintain in the long run.
Also, if I wanted to assert on all the fields, it would not be possible to treat this as an opaque type problem anymore, we would have to expose the inner structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't read too bad, but the types don't relate to anything in the code, which is confusing. Maybe using strings would fix that?
Following the problem specs makes sense.
|
||
type Stopwatch | ||
= Stopwatch | ||
{ state_ : State |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are the underscores for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because state
is an exposed function, so we are not allowed to shadow it. I was a bit lazy with the _
notation, that's the kind of thing you see in Haskell, but I didn't want to be too verbose.
Would you have a better suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen aState used before, which fits with it being an instance of the state type. theState sometimes as well.
|
||
|
||
start : Stopwatch -> Result String Stopwatch | ||
start (Stopwatch ({ state_ } as stopwatch)) = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could use a finite state machine here, instead of using Result (and in the other places)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to stick to the specs.
How would you write a finite state machine though? With phantom types? Or just by ignoring error instructions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked at the problem specs to be fair, but this code definitely looks like it should be one. All that error handling for the invalid states/actions is a definite code smell.
Maybe I could mention it on the forum and we can follow it up there if needs be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel that strongly about it, but you could check what other people think. I suspect it won't be that easy to change it, and they might suggest to create a new exercise based on finite state machines (which would be cool).
In the mean time, I'll keep it this way.
, Ok (PreviousLaps [ "01:23:45", "04:01:40" ]) | ||
, Ok (CurrentLap "08:43:05") | ||
, Ok (Total "14:08:30") | ||
, Ok (PreviousLaps [ "01:23:45", "04:01:40", "08:43:05" ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to make any test failures hard to read. Not sure what the other solutions are, but maybe asserting on all the fields all the time, instead of picking out one for each assertion?
Thank you for the review! |
This is a brand new one fresh from the problem specs.
Originally, I thought that would be an exercise about time, but really it's much more suited to be our first practice exercise about
opaque-types
.In reality,
Stopwatch
can be implemented as atype alias
, but I opted not to do that in the example solution to push students to use proper opaque types. Of course, we can't stop them from using type aliases, but we could add an analyzer check with some appropriate message. What do you think?